Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add template labels support to kubernetes labels #2473

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add template labels support to kubernetes labels #2473

wants to merge 7 commits into from

Conversation

theloneexplorerquest
Copy link
Contributor

@theloneexplorerquest theloneexplorerquest commented Apr 20, 2024 •
edited
Loading

Description

resolve #2310

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccKubernetesLabels'
==> Checking that code complies with gofmt requirements...
go vet ./...
TF_ACC=1 KUBE_CONFIG_PATH='/home/xxx/.kube/config' go test "/home/xxx/terraform-provider-kubernetes/kubernetes" -v === RUN   TestAccKubernetesLabels_basic
=== PAUSE TestAccKubernetesLabels_basic
=== RUN   TestAccKubernetesLabels_template_cronjob
=== PAUSE TestAccKubernetesLabels_template_cronjob
=== RUN   TestAccKubernetesLabels_template_deployment
=== PAUSE TestAccKubernetesLabels_template_deployment
=== RUN   TestAccKubernetesLabels_template_only
=== PAUSE TestAccKubernetesLabels_template_only
=== RUN   TestAccKubernetesLabels_resource_only
=== PAUSE TestAccKubernetesLabels_resource_only
=== CONT  TestAccKubernetesLabels_basic
=== CONT  TestAccKubernetesLabels_template_only
=== CONT  TestAccKubernetesLabels_template_deployment
=== CONT  TestAccKubernetesLabels_resource_only
=== CONT  TestAccKubernetesLabels_template_cronjob
--- PASS: TestAccKubernetesLabels_resource_only (12.26s)
--- PASS: TestAccKubernetesLabels_template_only (12.41s)
--- PASS: TestAccKubernetesLabels_template_deployment (15.62s)
--- PASS: TestAccKubernetesLabels_template_cronjob (16.85s)
--- PASS: TestAccKubernetesLabels_basic (16.90s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   16.930s

Release Note

Release note for CHANGELOG:

* `resource/kubernetes_labels`: Add a new attribute `template_labels`. [[GH-2310](https://github.com/hashicorp/terraform-provider-kubernetes/issues/2310)]

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@theloneexplorerquest theloneexplorerquest marked this pull request as ready for review April 20, 2024 13:14
@theloneexplorerquest theloneexplorerquest requested a review from a team as a code owner April 20, 2024 13:14
alexsomesan
alexsomesan reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a quick skim over this, seems OK so far.
One thing caught my eye - see my comment bellow.

I'll have a closer look and try it out when I get some spare cycles next week.

We'd benefit from a quick glance from @jrhouston too. He's more familiar with this part of the code than I am.

kubernetes/resource_kubernetes_labels.go
metadata = mmm
}
if l, ok := metadata["f:labels"].(map[string]interface{}); ok {
labels = l
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since labels is declared outside the for loop, this will overwrite it every time there is a match on manager. Should this rather be an append-type operation, adding the contents of l to labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since labels is declared outside the for loop, this will overwrite it every time there is a match on manager. Should this rather be an append-type operation, adding the contents of l to labels?

I have directly copied from template_annotations for this part, I will check for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since labels is declared outside the for loop, this will overwrite it every time there is a match on manager. Should this rather be an append-type operation, adding the contents of l to labels?

Hi @alexsomesan yeah, I agree there could be potential issues in theory, however, I wasn't able to reproduce it with kubectl.

I tried kubectl apply and kubectl patch on same object multiple times, however, looks like the server will store different manager kubectl-client-side-apply and kubectl-patch.

Do you have any suggestions on reproducing this issue? Or you guys are fine for me to change the code (without testing this case).

Copy link
Contributor

@sheneska sheneska May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @theloneexplorerquest, this issue is specific to using server-side-apply to access the API, however it appears you may be using client-side-apply as per the name of manager. kubectl by default uses client-side-apply and requires you to explicitly state --server-side in order to use the server-side-apply. Here's a useful link.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @theloneexplorerquest, this issue is specific to using server-side-apply to access the API, however it appears you may be using client-side-apply as per the name of manager. kubectl by default uses client-side-apply and requires you to explicitly state --server-side in order to use the server-side-apply. Here's a useful link.

Hi @sheneska thanks for reply. Yeah, I actually tried --server-side, however, I wasn't able to generate manifest with two field managers both named kubectl, do you have suggestion for that?

@BBBmau BBBmau mentioned this pull request Jul 19, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sheneska sheneska sheneska left review comments

@alexsomesan alexsomesan alexsomesan left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
documentation size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add template_labels support to kubernetes_labels
3 participants
@theloneexplorerquest @alexsomesan @sheneska